Skip to content

OCM Convert providerId/remoteId into string#35002

Merged
VicDeo merged 1 commit intomasterfrom
fix-34632
Apr 25, 2019
Merged

OCM Convert providerId/remoteId into string#35002
VicDeo merged 1 commit intomasterfrom
fix-34632

Conversation

@VicDeo
Copy link
Copy Markdown
Member

@VicDeo VicDeo commented Apr 10, 2019 •
edited
Loading

Description

providerId that we are storing in remoteId field should be a string according to the spec:

"providerId": "7c084226-d9a1-11e6-bf26-cec0c932ce01",

Related Issue

Motivation and Context

Better OCM spec support

How Has This Been Tested?

curl --header "Content-Type: application/json"   --request POST   --data \
'{"shareWith":"user@http:\/\/oc1","name":"\u0432\u0430\u0432\u0430.txt","providerId":"STRING","owner":"user2@http:\/\/oc2","ownerDisplayName":"manic","sender":"user2@http:\/\/oc2","senderDisplayName":"manic","shareType":"user","resourceType":"file","protocol":{"name":"webdav","options":{"sharedSecret":"pljNJU60e7ACFgW"}}}' \
http://oc1/index.php/apps/federatedfilesharing/shares

Before (internal server error)

{"message":"internal server error, was not able to add share from user2@http:\/\/oc2"}

After (share created)

[]

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • Backport (if applicable set "backport-request" label and remove when the backport was done)

@VicDeo VicDeo self-assigned this Apr 10, 2019
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 10, 2019 •
edited
Loading

Codecov Report

Merging #35002 into master will increase coverage by 0.01%.
The diff coverage is 75%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #35002      +/-   ##
============================================
+ Coverage     65.45%   65.46%   +0.01%     
- Complexity    18631    18633       +2     
============================================
  Files          1215     1216       +1     
  Lines         70545    70553       +8     
  Branches       1296     1296              
============================================
+ Hits          46174    46188      +14     
+ Misses        23994    23988       -6     
  Partials        377      377
Flag Coverage Δ Complexity Δ
#javascript 53.49% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.84% <75%> (+0.01%) 18633 <2> (+2) ⬆️
Impacted Files Coverage Δ Complexity Δ
apps/federatedfilesharing/lib/FedShareManager.php 80.12% <ø> (ø) 24 <0> (ø) ⬇️
apps/files_sharing/lib/External/Manager.php 76.54% <ø> (ø) 32 <0> (ø) ⬇️
apps/federatedfilesharing/lib/Notifications.php 11.53% <0%> (ø) 3 <0> (ø) ⬇️
lib/public/Share/Events/ShareEvent.php 40% <100%> (ø) 5 <0> (ø) ⬇️
...ederatedfilesharing/lib/FederatedShareProvider.php 62.66% <100%> (+1.52%) 85 <0> (ø) ⬇️
...aring/appinfo/Migrations/Version20190410160725.php 75% <75%> (ø) 2 <2> (?)
lib/private/Server.php 86.68% <0%> (+0.11%) 253% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f3d6c8...59e43bd. Read the comment docs.

@VicDeo VicDeo changed the title OCM Convert remote_id into string OCM Convert providerId/remoteId into string Apr 10, 2019
@VicDeo VicDeo added this to the development milestone Apr 16, 2019
@VicDeo VicDeo force-pushed the fix-34632 branch 2 times, most recently from d6907bf to 824d06d Compare April 17, 2019 20:40
@owncloud owncloud deleted a comment from codecov bot Apr 17, 2019
@VicDeo VicDeo force-pushed the fix-34632 branch 3 times, most recently from d89bd89 to 292a0f3 Compare April 24, 2019 13:07
@VicDeo VicDeo requested a review from PVince81 April 24, 2019 13:07
@PVince81 PVince81 requested a review from jvillafanez April 25, 2019 08:12
jvillafanez
jvillafanez approved these changes Apr 25, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@jvillafanez jvillafanez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks fine

@VicDeo VicDeo merged commit 432dd0c into master Apr 25, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix-34632 branch April 25, 2019 15:21
@VicDeo
Copy link
Copy Markdown
Member Author

VicDeo commented Apr 25, 2019

@PVince81 stable10?

@phil-davis
Copy link
Copy Markdown
Contributor

phil-davis commented Apr 26, 2019

@VicDeo
In update-testing https://drone.owncloud.com/owncloud/update-testing/398/188 https://drone.owncloud.com/owncloud/update-testing/398/134

2019-04-26T04:36:24+00:00 Doctrine\DBAL\Exception\DriverException: An exception occurred while executing 'ALTER TABLE oc_share_external ALTER remote_id TYPE BIGINT':

SQLSTATE[42804]: Datatype mismatch: 7 ERROR:  column "remote_id" cannot be cast automatically to type bigint
HINT:  You might need to specify "USING remote_id::bigint".
2019-04-26T04:36:24+00:00 Update failed
2019-04-26T04:36:24+00:00 Maintenance mode is kept active
2019-04-26T04:36:24+00:00 Reset log level

Those are postgresql update tests.
Why is it mentioning bigint when the migration apps/federatedfilesharing/appinfo/Migrations/Version20190410160725.php only mentions string ?

@VicDeo
Copy link
Copy Markdown
Member Author

VicDeo commented Apr 26, 2019

@phil-davis looks like migrations are executed in a wrong order. I'll have a look.

@VicDeo
Copy link
Copy Markdown
Member Author

VicDeo commented Apr 26, 2019

backport should include #35100

@phil-davis phil-davis mentioned this pull request Apr 26, 2019
Merged
11 tasks
@PVince81
Copy link
Copy Markdown
Contributor

PVince81 commented Apr 26, 2019

let's wait for update testing on Drone again: https://drone.owncloud.com/owncloud/update-testing

@VicDeo
Copy link
Copy Markdown
Member Author

VicDeo commented Apr 29, 2019

@PVince81 passing https://drone.owncloud.com/owncloud/update-testing

@PVince81
Copy link
Copy Markdown
Contributor

PVince81 commented Apr 29, 2019

@VicDeo please backport both to stable10 then.

I guess this will need a note in release notes regarding the DB change which might cause longer downtime during upgrade ?

@VicDeo VicDeo mentioned this pull request Apr 29, 2019
Merged
10 tasks
@VicDeo
Copy link
Copy Markdown
Member Author

VicDeo commented Apr 29, 2019

Stable10: #35122

@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jvillafanez jvillafanez jvillafanez approved these changes

@PVince81 PVince81 Awaiting requested review from PVince81

Assignees

@VicDeo VicDeo

Labels

3 - To Review

Projects

None yet

Milestone

development

Development

Successfully merging this pull request may close these issues.

OCM ProviderId as int instead of string

4 participants

@VicDeo @phil-davis @PVince81 @jvillafanez